Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[FE] 생성한 행사 모아보기 페이지 #856

Merged
merged 7 commits into from
Dec 18, 2024
Merged

Conversation

Todari
Copy link
Contributor

@Todari Todari commented Dec 16, 2024

issue

구현 목적

  • 로그인 기능이 생긴 이후, 유저들이 지금까지 본인이 생성한 행사를 모아볼 수 있게 되었습니다.
  • 사용자가 지금까지 정산을 진행한 페이지를 한눈에 모아볼 수 있도록 해당 페이지를 구현하여 제공합니다.

구현 사항

  • route는 기존 @jinhokim98 이 구현한 mypage를 이용하여, mypage/events 로 정했습니다.

  • 기존의 EventHomePage과 비슷한 UI, 기능들로 인하여 이를 참고하여 구현하였습니다.

  • 피그마의 UX writing을 일부 수정하였습니다

  • 기존 API는 오래된 것을 먼저 보여주지만, useRequestGetCreatedEvents hook 내부에서 data를 최신순으로 sort 하여 반환합니다.
    image

  • queryKey에 eventId 추가

  • 기존 방식대로면, eventId가 변경되면서 다른 페이지에 접근해도, 기존의 요청 캐시로 인해 정보가 갱신되지 않았습니다. 이에 get 요청들의 query key에 eventId를 추가해 줌으로써, 페이지가 변경될 때 마다 새로 요청을 보내도록 변경했습니다.

  • 개선 전

2024-12-16.7.14.31.mov
  • 개선 후
2024-12-16.7.18.01.mov

논의하고 싶은 부분(선택)

1. Component 재사용성에 대한 논의

기존의 ExpenseList와 비슷한 UI라고 생각하여 이를 재사용하려 하였으나, 이 조차도Event엔티티에 깊게 결합되어있어 재사용할 수 없었습니다.
따라서, 비슷한 UI와 style 부분을 별도로 복사하여 작업하게 되었습니다.
계속 프로젝트가 진행될 때 마다, 이미 존재하는 디자인 요소를 그대로 사용함으로 인해서 확실히 생산성이 충분히 많이 향상되어 있다고 생각합니다.
하지만 반대로, 이와 같이 복사해서 사용하게 된다면 유지보수에 큰 어려움이 존재할 수 밖에 없다고 생각합니다.
앞으로 프로젝트를 얼마나 지속할지도, 디자인의 변경이 생겨 유지보수할 일도 예측이 불가능 하지만, 확실히 component들의 재사용성을 높여둔다면 지금까지 어느정도 필요한 component들이 모두 나온 상황에서 정말 최상의 재사용성을 유지할 수 있다고 생각합니다.

2.Page 폴더 디렉토리 및 route에 관한 논의

확실히 우리 서비스의 페이지가 많아지다 보니, 원하는 페이지 파일을 손쉽게 찾기 힘들어지고 있다고 느껴집니다.
Pages 디렉토리에 정말 많은 Page tsx 파일들이 있는데, 이를 route와 통일하여 논리적으로 쉽게 접근할 수 있는 방향을 함께 논의해보면 어떨까 싶습니다. 이런 작은 convention 하나하나가 모여 높은 생산성을 가진 협업의 기초가 되지 않을까 싶습니다...!
만약 논의가 되고 난 후, 새로운 페이지가 생기는 task를 가져갈 때, 해당 페이지의 route 및 directory를 함께 정하고 작업에 들어가도 좋을 것 같습니다!

결론

  1. 많은 디자인 요소가 결정된 상황에서, 손쉬운 기능 추가 및 유지보수를 위해 component 재사용성을 높이고 싶다.
  2. 쉬운 page 파일을 찾아내기 위해 route path, /Page 디렉토리, page 파일 이름에 대한 컨벤션을 정하고 싶다.

🫡 참고사항

@Todari Todari added 🖥️ FE Frontend ⚙️ feat feature labels Dec 16, 2024
@Todari Todari added this to the v3.1.0 milestone Dec 16, 2024
@Todari Todari self-assigned this Dec 16, 2024
Copy link

Copy link

Copy link

Copy link
Contributor

@jinhokim98 jinhokim98 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

바쁜 와중에도 뚝딱 기능을 만들어내는 모습 멋있습니다~~ 고생했어요.
토다리가 논의해주신 재사용성 동의합니다. 토다리의 코드를 리뷰하면서 확실히 css 복사 붙여넣기한 내용이 많은 것 같더라구요... 제가 이해한 토다리가 원하는 재사용성을 높이는 방식은 디자인을 가진 컴포넌트에서 도메인 결합도를 낮추는 것이라고 이해했는데 맞을까요? 어떻게 해야할 지 논의를 해봐야겠지만 개발을 지속적으로 이어나간다고 하면 작업을 하는 것이 좋다고 생각해요!
Page 폴더도 확실히 정리가 되지 않은 느낌이 들어요. 이 정리는 위 정리보다 금방 할 것 같으니 회의 끝나고 아니면 회의 도중에 정리를 해서 그 상태에서 다시 개발을 시작해도 좋을 것 같습니다!

Comment on lines 33 to 40
<Flex
justifyContent="spaceBetween"
alignItems="center"
height="2.5rem"
padding="0.5rem 1rem"
paddingInline="0.5rem"
>
<Flex gap="0.5rem" alignItems="center" onClick={onClick}>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

확실히 그래요.. 이런 스타일을 복사 붙여 넣기 하면서 유지 보수가 어려워진다는 점 공감합니다.
이런 자주 사용하는 레이아웃을 따로 정의해서 가져온다면 개선할 수 있지 않을까 생각합니다.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

추가로 여기 Flex 컴포넌트의 onClick 궁금한 점이 있습니다.
onClick의 범위가 아래 사진 영역만큼인가요?

이미지 설명

아니면 가로 전체인가요?

이미지 설명

토다리는 어떤 경우가 사용자에게 더 편한 경험을 줄 것이라고 생각하는지 궁금합니다!

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

오호 아래 영역이 더 나을 것이라고 판단되네요.
위에 onClick을 넣도록 변경하겠습니다~

95b2fae


const useRequestGetCreatedEvents = () => {
const {data, ...rest} = useQuery({
queryKey: [QUERY_KEYS.createdEvents],
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

유저 로그아웃이 없으니깐 토다리가 작성해준 대로 이 부분은 queryKey 배열의 두 번째 인자 신경 쓰지 않아도 될 것 같아요!

queryFn: () => requestGetCreatedEvents(),
select: data => ({
...data,
events: data.events.sort((a, b) => new Date(b.createdAt).getTime() - new Date(a.createdAt).getTime()),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

select 좋은 것 같아요! 이 시간 비교는 밀리초 단위로 비교되는 건가요?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

아뇨 10^-6 까지 비교합니다 ㅋㅋ

@@ -11,7 +11,7 @@ const useRequestGetEvent = ({...props}: WithErrorHandlingStrategy | null = {}) =
const eventId = getEventIdByUrl();

const {data, ...rest} = useSuspenseQuery({
queryKey: [QUERY_KEYS.event],
queryKey: [QUERY_KEYS.event, eventId],
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

원래는 이벤트 생성을 했을 때 removeQueries를 사용해서 캐시를 전부 날려주었기 때문에 문제가 없었는데, 이제는 생성 외에 다른 이벤트 접근이 가능해서 일어난 문제였네요.. 이것 외에 또 다른 부작용은 없었나요?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

우선 손수 테스트 한 케이스에선 별도로 없었습니다~!

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

오호 좋아요~

Copy link
Contributor

@pakxe pakxe left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

고생많으셨씁니다!! 이쁘게 구현이 되었네요!~~

토달이 pr에 써있는 대로 파일을 뷰어에서 찾기 어렵다는 것 이해합니당. 저는 특히 훅찾을 때가 어렵더라구요. 정리하면 좋겠네요~ 생각이 드는건 특정 페이지에서만 쓰이는 훅이나 컴포넌트를 함께 뭉쳐두는게 어떨까해요. 거기에서만 쓰이는건데 나와있으니까 다른 파일 찾는데 더 복잡해진 느낌이라..

Copy link

Copy link

Copy link
Contributor

@soi-ha soi-ha left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

구현해주신 부분 잘 봤어요! 꼼꼼하게 해주신 것 같아서 남길 코멘트는 딱히 없네요 ㅎㅎ 대신 토다리가 논의 요청한 부분에 대한 저의 생각을 말해드리자면..

많은 디자인 요소가 결정된 상황에서, 손쉬운 기능 추가 및 유지보수를 위해 component 재사용성을 높이고 싶다.

토다리가 말씀해주신 것 처럼 앞으로의 디벨롭을 생각하면 component 재사용성을 높이는 부분은 적극 찬성입니다. 그리고 저번에 우리가 디자인시스템을 폐기하면서 컴포넌트 폴더 구조가 약간? 이상한 것도 없지 않아 있구요. 다른 위치지만 컴포넌트 폴더가 두개 존재하니.. 이 부분은 추후 생성되는 기능이 없을 때 진행하면 정말 좋을 것 같아요!

쉬운 page 파일을 찾아내기 위해 route path, /Page 디렉토리, page 파일 이름에 대한 컨벤션을 정하고 싶다.

이 부분은 정말 정말 필요하다고 생각합니다.. 아무래도 route와 Page 폴더의 파일 네이밍이 다르다보니 찾는게 어렵더라구요...ㅜ 이 부분은 빠른 시일내에.. 진행하면 좋을 것 같습니댱!

height="100%"
cssProp={{borderRadius: '1rem'}}
>
<Input inputType="search" value={eventName} onChange={onSearch} placeholder={placeholder} />
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

원하는 행사 서칭까지 가능하도록 해주시다니~ 👍

@@ -0,0 +1,12 @@
import {CreatedEvent} from './../../../../types/serviceType';
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

types/serviceType로 import 할 수 없었나요?.?

@@ -11,7 +11,7 @@ const useRequestGetSteps = ({...props}: WithErrorHandlingStrategy | null = {}) =
const eventId = getEventIdByUrl();

const queryResult = useQuery({
queryKey: [QUERY_KEYS.steps],
queryKey: [QUERY_KEYS.steps, eventId],
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

오우.. 다른 부분도 꼼꼼히 체크해서 eventId 넣어주셨군요! 쵝오

@jinhokim98 jinhokim98 merged commit 9e583d9 into fe-dev Dec 18, 2024
2 checks passed
@jinhokim98 jinhokim98 deleted the feature/#854 branch December 18, 2024 07:29
Copy link

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
Status: ✅ Done
Development

Successfully merging this pull request may close these issues.

4 participants